Remove game import from Container, default dimensions to Infinity#1348
Remove game import from Container, default dimensions to Infinity#1348
Conversation
Container constructor no longer imports or references the global game singleton. Default width/height changed from game.viewport dimensions to Infinity (no clipping). This is the same effective behavior since all actual Container creations pass explicit dimensions. Updated JSDoc to document Infinity defaults. Added tests: default dimensions, position defaults, explicit dimensions, children with Infinity container, no-clip behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR decouples Container from the global game singleton by removing the game.viewport-based default sizing, and instead defaults container dimensions to Infinity to avoid implicit clipping when dimensions aren’t provided.
Changes:
- Removed
gameimport/usage fromContainerand simplified the constructor to pass throughwidth/heightdirectly. - Changed default
Containerwidth/height toInfinityand updated the constructor JSDoc accordingly. - Added tests covering default dimensions and some basic behaviors when dimensions are
Infinity.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/melonjs/src/renderable/container.js | Removes dependency on game.viewport and defaults container dimensions to Infinity. |
| packages/melonjs/tests/container.spec.js | Adds test cases for default Container dimensions and related expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Container: Infinity defaults, isFinite() guard in updateBounds, anchorPoint (0,0) - ImageLayer: uses parentApp for viewport/renderer, deferred to onActivateEvent - TMXTileMap: resolves viewport from container tree via getRootAncestor().app - Input: pointer/pointerevent receive app via GAME_INIT instead of game singleton - Stage: onDestroyEvent(app) now passes Application instance - video.js: DOM event listeners moved to Application.init() - video.renderer, video.init(), video.getParent(), device.onReady() deprecated - Application: improved JSDoc, DOM event bridge for resize/orientation/scroll - BitmapTextData: fix circular import (pool.ts <-> bitmaptextdata.ts) - All me.game.* JSDoc references updated to app.* - README hello world updated to use new Application() pattern - Platformer and UI examples migrated to Application entry point - New tests: container (Infinity/isFinite/clipping), ImageLayer (parentApp) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…string Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const bounds = this.getBounds(); | ||
|
|
||
| // call parent method | ||
| super.updateBounds(absolute); | ||
| if (this.isFinite()) { | ||
| // call parent method only when container has finite dimensions | ||
| super.updateBounds(absolute); |
There was a problem hiding this comment.
For Infinity-sized containers, updateBounds() skips super.updateBounds() but does not clear/reset the existing bounds before aggregating child bounds. Since non-finite bounds are initialized with max = Infinity, subsequent bounds.addBounds(childBounds) will keep the container bounds infinite and won’t become child-derived. Clear/reset bounds (or start from the first child bounds with clear=true) when !this.isFinite() and enableChildBoundsUpdate is enabled.
| // called when the layer is added to the game world or a container | ||
| onActivateEvent() { | ||
| // register to the viewport change notification | ||
| const viewport = this.parentApp.viewport; | ||
| // set the initial size to match the viewport | ||
| this.resize(viewport.width, viewport.height); | ||
| this.createPattern(); |
There was a problem hiding this comment.
ImageLayer.onActivateEvent() dereferences this.parentApp immediately. Container.addChild() invokes onActivateEvent even when the container tree is not attached to a root/world container, so parentApp may be unavailable and this will throw. Add a guard (throw a clear error or defer initialization until attached) before using parentApp.viewport/renderer.
| if (pointerEventTarget === null || pointerEventTarget === undefined) { | ||
| // default pointer event target | ||
| pointerEventTarget = game.renderer.getCanvas(); | ||
| pointerEventTarget = _app.renderer.getCanvas(); | ||
| } |
There was a problem hiding this comment.
enablePointerEvent() uses _app.renderer.getCanvas() as the default event target, but _app is only assigned via the GAME_INIT event. If bindPointer() / registerPointerEvent() is called before an Application is constructed, this will crash. Add a defensive check and throw a clear initialization error when _app is undefined.
| // bridge DOM events to the melonJS event system | ||
| globalThis.addEventListener("resize", (e) => { | ||
| emit(WINDOW_ONRESIZE, e); | ||
| }); | ||
| globalThis.addEventListener("orientationchange", (e) => { |
There was a problem hiding this comment.
The constructor registers global resize/orientationchange/scroll listeners via globalThis.addEventListener(...), but destroy() does not remove them. Creating/destroying multiple Application instances will leak listeners and duplicate emitted events. Store handler references and unregister them in destroy().
| const app = container.getRootAncestor().app; | ||
|
|
||
| function _setBounds(width, height) { |
There was a problem hiding this comment.
TMXTileMap.addTo() resolves const app = container.getRootAncestor().app; unconditionally. If container is not attached to a root/world container, getRootAncestor() can return undefined and this will throw even when setViewportBounds is false. Move app resolution inside the setViewportBounds === true branch and add a defensive check when no root/app is available.
| const app = container.getRootAncestor().app; | |
| function _setBounds(width, height) { | |
| function _setBounds(width, height) { | |
| const rootAncestor = container.getRootAncestor(); | |
| const app = rootAncestor && rootAncestor.app; | |
| if (!app || !app.viewport) { | |
| return; | |
| } |
- Container: clear bounds before child aggregation for infinite containers - ImageLayer: throw if parentApp unavailable in onActivateEvent - Input: throw if Application not initialized when enabling pointer events - Application: store DOM event handlers, remove them in destroy() - TMXTileMap: move app resolution inside setViewportBounds branch Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| globalThis.addEventListener("resize", this._onResize); | ||
| globalThis.addEventListener("orientationchange", this._onOrientationChange); | ||
| if (device.screenOrientation) { | ||
| globalThis.screen.orientation.onchange = this._onOrientationChange; |
There was a problem hiding this comment.
Application.init() assigns screen.orientation.onchange directly and destroy() unconditionally clears it. This can clobber another Application instance’s handler (or any consumer handler) when multiple apps exist or when an app is destroyed out of order. Prefer screen.orientation.addEventListener('change', ...)/removeEventListener, or only clear onchange if it still equals this instance’s handler (restore previous handler otherwise).
| globalThis.screen.orientation.onchange = this._onOrientationChange; | |
| globalThis.screen.orientation.addEventListener( | |
| "change", | |
| this._onOrientationChange, | |
| ); |
| this._onOrientationChange!, | ||
| ); | ||
| globalThis.removeEventListener("scroll", this._onScroll!); | ||
| if (device.screenOrientation) { |
There was a problem hiding this comment.
destroy() sets screen.orientation.onchange = null without checking whether another Application (or user code) has since installed a different handler. This can break orientation handling in multi-app scenarios. Only clear it if the current value matches this instance’s handler, or switch to add/removeEventListener for ScreenOrientation change events.
| if (device.screenOrientation) { | |
| if ( | |
| device.screenOrientation && | |
| globalThis.screen.orientation.onchange === this._onOrientationChange | |
| ) { |
| @@ -123,15 +119,23 @@ export default class ImageLayer extends Sprite { | |||
| this.repeatY = true; | |||
| break; | |||
| } | |||
There was a problem hiding this comment.
The repeat setter no longer triggers a resize / pattern rebuild. If repeat is changed at runtime after activation, width/height and _pattern can become inconsistent until a viewport resize happens. Consider updating size (this.resize(viewport.width, viewport.height)) and recreating the pattern when parentApp is available and the layer is active.
| } | |
| } | |
| if (this.parentApp && this.parentApp.viewport) { | |
| const viewport = this.parentApp.viewport; | |
| this.resize(viewport.width, viewport.height); | |
| this.createPattern(); | |
| } |
| * In this case, anchorPoint is treated as (0, 0) since there is no meaningful | ||
| * center for an infinite area. Bounds are then derived entirely from children | ||
| * when {@link Container#enableChildBoundsUpdate} is enabled. |
There was a problem hiding this comment.
The doc/comment says the container anchorPoint is “always” (0,0) / “treated as” (0,0), but nothing prevents users from changing anchorPoint after construction (which can reintroduce Infinity * anchor issues). Consider clarifying this as a default (not an invariant), or enforcing it (e.g. override setter / clamp to 0 when width/height are non-finite).
| * In this case, anchorPoint is treated as (0, 0) since there is no meaningful | |
| * center for an infinite area. Bounds are then derived entirely from children | |
| * when {@link Container#enableChildBoundsUpdate} is enabled. | |
| * In this case, the default anchorPoint should remain at (0, 0), since there is | |
| * no meaningful center for an infinite area. Bounds are then derived entirely | |
| * from children when {@link Container#enableChildBoundsUpdate} is enabled. |
| const app = new Application(1218, 562, { | ||
| parent: "screen", | ||
| scale: "auto", | ||
| backgroundColor: "#202020", | ||
| }); | ||
|
|
||
| // set a gray background color |
There was a problem hiding this comment.
In this example, backgroundColor passed to new Application(...) only affects the parent element CSS background (per ApplicationSettings docs), while app.world.backgroundColor.parseCSS(...) controls the rendered clear color. Having both (and the comment “set a gray background color”) is confusing—consider either removing the option or adjusting the comments to reflect the distinction.
| const app = new Application(1218, 562, { | |
| parent: "screen", | |
| scale: "auto", | |
| backgroundColor: "#202020", | |
| }); | |
| // set a gray background color | |
| // `backgroundColor` here sets the parent element CSS background | |
| const app = new Application(1218, 562, { | |
| parent: "screen", | |
| scale: "auto", | |
| backgroundColor: "#202020", | |
| }); | |
| // set the rendered world clear color to gray |
| /** | ||
| * Initialize the melonJS library. | ||
| * This is called automatically in two cases: | ||
| * - On DOMContentLoaded, unless {@link skipAutoInit} is set to true | ||
| * - By {@link Application.init} when creating a new game instance | ||
| * | ||
| * This is called automatically by the {@link Application} constructor. | ||
| * Multiple calls are safe — boot() is idempotent. | ||
| * @see {@link skipAutoInit} | ||
| * When using {@link Application} directly, calling boot() manually is not needed. | ||
| * @see {@link Application} |
There was a problem hiding this comment.
The updated JSDoc for boot() says it’s “called automatically by the Application constructor”, but the entrypoint still auto-calls boot() on DOMContentLoaded when skipAutoInit is false (see src/index.ts). Consider updating the doc to reflect both auto-init paths (DOMContentLoaded + Application.init/constructor) so users relying on skipAutoInit aren’t misled.
Summary
gamesingletongame.viewportlookup toInfinity(no clipping)InfinitydefaultsTest plan
🤖 Generated with Claude Code